fix(select): Select component keyboard type-ahead#10006
Conversation
|
Looks like it's still failing tests, how would you like to handle those? Do you need assistance? Also, it looks like you haven't signed the CLA, see https://github.com/adobe/react-spectrum/blob/main/CONTRIBUTING.md#contributor-license-agreement |
|
@snowystinger I already signed though the email notification. but still not passing. I need your help on that. |
|
I've updated with a merge from main, hopefully that fixes the remaining two test failures that I don't think were your fault. As for the CLA, could you try again and double check the e-mail used. This happens pretty frequently, especially when people have multiple accounts/emails/etc. It doesn't look like the service is down as other PRs are being successfully checked. |
|
Passed all the quality gate issues @snowystinger. please review this PR |
withlakshmanan
force-pushed
the
fix/select-type-a-head-issue
branch
from
c20a67e to
f5fe02e
Compare
|
@reidbarber @snowystinger @devongovett Please review this PR. I fixed all the quality gate issues. some check fails due to reverse merge main branch. |
|
Thanks for being patient, we are looking for some time to review. |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Thanks for your patience
Looks like we want it to match the behaviour of native #8919 (comment)
I think we have some tests that are currently wrong, so we should update the behaviour.
I think this should be on option[5] after pressing F from the first item since that was already Foo.
Here's the implementation of Chrome's https://github.com/chromium/chromium/blob/main/third_party/blink/renderer/core/html/forms/type_ahead.cc
We can probably ignore their part about the time delta, we already implement that with the timer. And we can defer text value matching to the delegates.
| await user.tab(); | ||
| await user.keyboard('bb'); | ||
| expect(trigger).toHaveTextContent('Blackberry'); | ||
| }); |
There was a problem hiding this comment.
it('should wrap if typeahead is not found after the current key', async function () {
let {getByTestId} = render(
<Select defaultSelectedKey="blueberry" data-testid="select">
<Label>Favorite Fruit</Label>
<Button>
<SelectValue />
</Button>
<Popover>
<ListBox>
<ListBoxItem id="banana">Banana</ListBoxItem>
<ListBoxItem id="blackberry">Blackberry</ListBoxItem>
<ListBoxItem id="blueberry">Blueberry</ListBoxItem>
</ListBox>
</Popover>
</Select>
);
let wrapper = getByTestId('select');
let selectTester = testUtilUser.createTester('Select', {
root: wrapper,
interactionType: 'keyboard'
});
let trigger = selectTester.getTrigger();
await user.tab();
await user.keyboard('b');
expect(trigger).toHaveTextContent('Banana');
});
// This matches the three main browsers behavior. Even though it seems like it should go to double "b" blackberry,
// it's just cycling through the items that start with "b".
it('searches the next item that starts with the same letter, not the next item that starts with the same letter twice', async function () {
let {getByTestId} = render(
<Select data-testid="select">
<Label>Favorite Fruit</Label>
<Button>
<SelectValue />
</Button>
<Popover>
<ListBox>
<ListBoxItem id="banana">Banana</ListBoxItem>
<ListBoxItem id="boisenberry">Boisenberry</ListBoxItem>
<ListBoxItem id="bblackberry">Bblackberry</ListBoxItem>
<ListBoxItem id="blueberry">Blueberry</ListBoxItem>
</ListBox>
</Popover>
</Select>
);
let wrapper = getByTestId('select');
let selectTester = testUtilUser.createTester('Select', {
root: wrapper,
interactionType: 'keyboard'
});
let trigger = selectTester.getTrigger();
await user.tab();
await user.keyboard('bb');
expect(trigger).toHaveTextContent('Boisenberry');
});
|
|
||
| key = this.getNextKey(key); | ||
|
|
||
| if (key == null && !hasWrapped) { |
There was a problem hiding this comment.
shouldn't need to do wrapping here, i think we already handle it in useTypeSelection, if a key wasn't found after the starting point, we try again from the top of the list
|
|
||
| let collection = this.collection; | ||
| let key = fromKey || this.getFirstKey(); | ||
| let key = fromKey != null ? this.getNextKey(fromKey) : this.getFirstKey(); |
There was a problem hiding this comment.
I think it'd be better if we can handle this in useTypeSelect instead, it may affect unrelated implementations, and we'd need to go make the change in every delegate and layout which implements this function probably
| if ( | ||
| !character || | ||
| e.ctrlKey || | ||
| e.metaKey || |
There was a problem hiding this comment.
looks like we're missing an e.altKey || check according to the chrome implementation
2 participants
Closes #8919
Description
The fix makes Select keyboard typeahead behave correctly for repeated letters and ensures search continues from the next option rather than reusing the current focused item.
Screen recording
Screen.Recording.2026-04-25.at.1.58.40.PM.mov
Issue link
#8919
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: